fix: visualizer getCSV method throwing constant errors#1263
fix: visualizer getCSV method throwing constant errors#1263HardeepAsrani wants to merge 1 commit intodevelopmentfrom
Conversation
pirate-bot
added
the
pr-checklist-complete
There was a problem hiding this comment.
Pull request overview
This PR aims to stop Visualizer’s CSV export (_getCSV) from repeatedly triggering runtime errors by adding additional safeguards around temporary file creation, and updates the test suite/tooling to support newer PHPUnit behavior.
Changes:
- Add a fallback/early-return guard in
Visualizer_Module::_getCSV()when a writable temp file handle can’t be created. - Refactor/export tests to reuse a helper for running the export AJAX action and add CSV response-structure assertions.
- Add Yoast PHPUnit Polyfills (and bootstrap loading) to improve PHPUnit cross-version compatibility; update
composer.lockaccordingly.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
classes/Visualizer/Module.php |
Adds additional guard(s) when temp file creation fails during CSV generation. |
tests/test-export.php |
Adds a helper to run export AJAX, adds CSV structure test, adjusts invalid-chart expectations. |
tests/test-ajax.php |
Replaces deprecated PHPUnit assertions with property_exists() checks. |
tests/bootstrap.php |
Loads Yoast PHPUnit Polyfills autoloader early in the test bootstrap. |
composer.json |
Adds yoast/phpunit-polyfills to require-dev. |
composer.lock |
Locks new dev dependencies brought in by PHPUnit polyfills/tooling changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $fp = fopen( wp_tempnam(), 'w+' ); | ||
| } | ||
| if ( ! $fp ) { | ||
| return array( 'csv' => '', 'name' => $filename, 'string' => '' ); |
There was a problem hiding this comment.
_getCSV() now returns a non-empty array even when it cannot obtain a writable temp file handle. Because callers (e.g. the AJAX export handler) typically treat any non-empty return value as success, this can result in success: true responses with an empty CSV payload, which is misleading and makes failures harder to detect. Consider returning null/false on file-handle failure (or including an explicit error indicator) so the export endpoint can fail gracefully (e.g. send a JSON error) instead of reporting success with empty data.
| return array( 'csv' => '', 'name' => $filename, 'string' => '' ); | |
| return null; |
There was a problem hiding this comment.
For this, do we have a handler that works with an empty string property? Or they just download an empty file?
| /** | ||
| * Runs the export AJAX action and returns the decoded response. | ||
| * | ||
| * @since 3.11.0 | ||
| * | ||
| * @access private | ||
| * @return object | ||
| */ | ||
| private function run_export() { | ||
| $_GET = array( | ||
| 'security' => wp_create_nonce( Visualizer_Plugin::ACTION_EXPORT_DATA . Visualizer_Plugin::VERSION ), | ||
| 'chart' => $this->chart, | ||
| ); | ||
|
|
||
| ob_start(); | ||
| try { | ||
| $this->_handleAjax( 'visualizer-export-data' ); | ||
| } catch ( WPAjaxDieContinueException $e ) { | ||
| // We expected this, do nothing. | ||
| } catch ( WPAjaxDieStopException $ee ) { | ||
| // We expected this, do nothing. | ||
| } | ||
| ob_end_clean(); | ||
|
|
||
| return json_decode( $this->_last_response ); | ||
| } |
There was a problem hiding this comment.
The run_export() docblock says @return object, but json_decode() can return null on empty/invalid JSON. Updating the return type doc (and/or adding an assertion that $this->_last_response is non-empty) will make the helper’s contract accurate and prevent confusing failures when the AJAX handler emits no JSON.
There was a problem hiding this comment.
I think this is a good observation.
| $this->create_chart(); | ||
| $this->_setRole( 'administrator' ); | ||
|
|
||
| $response = $this->run_export(); |
There was a problem hiding this comment.
test_csv_export_response_structure() dereferences $response->success without first asserting that $response is an object. If the AJAX handler returns no output (e.g. due to an environment issue), this will error out with a PHP fatal instead of producing a clear assertion failure. Consider adding assertIsObject( $response ) (and optionally property_exists) before using $response->success.
| $response = $this->run_export(); | |
| $response = $this->run_export(); | |
| $this->assertIsObject( $response ); | |
| $this->assertTrue( property_exists( $response, 'success' ) ); | |
| $this->assertTrue( property_exists( $response, 'data' ) ); |
Soare-Robert-Daniel
left a comment
Soare-Robert-Daniel
left a comment
There was a problem hiding this comment.
I think Copilot made some great points on the review.
| /** | ||
| * Runs the export AJAX action and returns the decoded response. | ||
| * | ||
| * @since 3.11.0 | ||
| * | ||
| * @access private | ||
| * @return object | ||
| */ | ||
| private function run_export() { | ||
| $_GET = array( | ||
| 'security' => wp_create_nonce( Visualizer_Plugin::ACTION_EXPORT_DATA . Visualizer_Plugin::VERSION ), | ||
| 'chart' => $this->chart, | ||
| ); | ||
|
|
||
| ob_start(); | ||
| try { | ||
| $this->_handleAjax( 'visualizer-export-data' ); | ||
| } catch ( WPAjaxDieContinueException $e ) { | ||
| // We expected this, do nothing. | ||
| } catch ( WPAjaxDieStopException $ee ) { | ||
| // We expected this, do nothing. | ||
| } | ||
| ob_end_clean(); | ||
|
|
||
| return json_decode( $this->_last_response ); | ||
| } |
There was a problem hiding this comment.
I think this is a good observation.
| $fp = fopen( wp_tempnam(), 'w+' ); | ||
| } | ||
| if ( ! $fp ) { | ||
| return array( 'csv' => '', 'name' => $filename, 'string' => '' ); |
There was a problem hiding this comment.
For this, do we have a handler that works with an empty string property? Or they just download an empty file?
Summary
Adds safeguards to prevent the
_getCSVfrom throwing constant errors.Will affect visual aspect of the product
YES/NO
Screenshots
Test instructions
Check before Pull Request is ready:
Closes https://github.com/Codeinwp/visualizer-pro/issues/400.